Fixes for --stop-before flag.
#1667
Conversation
|
@kacf, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline". my commands and optionsYou can trigger a pipeline on multiple prs with:
You can start a fast pipeline, disabling full integration tests with:
You can trigger GitHub->GitLab branch sync with:
You can cherry pick to a given branch or branches with:
|
|
Must be merged together with mendersoftware/meta-mender#2171. |
There was a problem hiding this comment.
See my comment wrt ArtifactCommit_Leave below.
Also, I think you got confused by testing this PR with mendersoftware/meta-mender#2171 - what we need to fix for this PR is mender-image-tests use of this new flag.
| "--stop-after", | ||
| "ArtifactCommit", | ||
| "--stop-before", | ||
| "ArtifactCommit_Leave", |
There was a problem hiding this comment.
What is the difference between stopping before ArtifactCommit_Leave or Cleanup? For the successful case, there is no real difference. And yes, for failure it will behave different (it would stop or not), but maybe we could merge these two use cases and stop only before Cleanup?
From the API point of view, to allow stopping before a Leave state sounds strange. Note that every other allowed state to stop before are Enter states (or Cleanup)
There was a problem hiding this comment.
There is a big difference actually, but I also didn't realize it until I started looking at this in great detail. ArtifactCommit_Leave state scripts are only executed when the commit has definitely happened, and will not roll back (this is documented here). In other words, when executing ArtifactCommit_Leave scripts, we need to be sure that we will not roll back afterwards, hence we need to stop before them, and not before Cleanup, at which point it would be too late already. Rolling back after a successful commit, but before the Leave scripts, is possible with the orchestrator because other payloads may fail to commit in between.
The state machine is indeed quite complicated in this respect, and I can't say I particularly like this complexity. But it underlines how hard it is to both execute all states correctly, including on powerloss, while simultaneously offering a command line interface to fine control those states. It's not easy! 😃
Well, kind of. I did actually want to test this with that PR just to make sure I hadn't broken anything unintentionally. But you're right it's not directly related. In any case, it's the orchestrator which needs the symlink change, in its integration tests. I have tested it locally, but the PR for that comes a little bit later. |
This is done for two main reasons: First, it is more intuitive to name the state you want to stop before instead of the ones you want to stop after. For example, multiple states lead to `ArtifactRollback_Enter`; with the `--stop-after` flag you'd have to specify both `ArtifactInstall_Error` and `ArtifactCommit_Error`. Second, in the orchestrator it is not a good fit to use `--stop-after`, for reasons explained in the corresponding commits there, and renaming keeps them more in sync. This has little effect on how it works, it's just using different names, so both tests and usage stay nearly the same, just name-adjusted. Cancel-changelog: b60beaf Cancel-changelog: 2c1d922 Changelog: Add `--stop-before` flag which can be used with the `install`, `commit`, and `rollback` standalone commands to stop before certain states. Use `resume` to continue, which also supports the same flag. These are the allowed states: * `ArtifactInstall_Enter` * `ArtifactCommit_Enter` * `ArtifactCommit_Leave` * `ArtifactRollback_Enter` * `ArtifactFailure_Enter` * `Cleanup` The flag can be specified multiple times. Ticket: MEN-7115 Signed-off-by: Kristian Amlie <[email protected]>
This allows us to be a bit more selective when deciding whether or not to print it, which helps in upcoming commits. Signed-off-by: Kristian Amlie <[email protected]>
Changelog: Title Ticket: None Signed-off-by: Kristian Amlie <[email protected]>
They have no effect on the flow anyway. Signed-off-by: Kristian Amlie <[email protected]>
First and foremost, make sure that repeated resuming before the same state, with the same `--stop-before` flag, is a noop. It should keep stopping there. Also make sure that once we have started executing the path after that, it should no longer be possible to go back. For this we introduce some new DB flags, but since the existing ones have not been released yet, this should be fine. Signed-off-by: Kristian Amlie <[email protected]>
|
Merging these commits will result in the following changelog entries: Changelogsmender (rename)New changes in mender since master: Bug Fixes
Features
|
|
Review comments addressed, and tests added for the last commit! |
|
Thanks! |
|
Hello 😺 This PR contains changelog entries. Please, verify the need of backporting it to the following release branches: |
No description provided.